Group field caps shard requests per node #77047

jtibshirani · 2021-08-31T06:05:39Z

Currently to gather field caps, the coordinator sends a separate transport
request per index. When the original request targets many indices, the overhead
of all these sub-requests can add up and hurt performance. This PR switches the
execution strategy to reduce the number of transport requests: it groups
together the index requests that target the same node, then sends only one
request to each node.

Addresses #74648.

jtibshirani · 2021-08-31T22:56:27Z

This approach makes compromises to keep the implementation simple. I'm looking for feedback on these decisions:

Before, if a shard copy unexpectedly failed while retrieving field caps, we retried on the next copy. Now that index will just be marked as "failed" in the response. This seemed okay to me, I wasn't sure we needed this kind of "live retry" logic in field caps?
Previously when there was an index_filter, we tried each shard in the index one after the other, stopping when one of them matched. Now we fan out to all of the index shards preemptively, which means the nodes do redundant work.

Also -- I opened this PR against 7.x since the BWC logic can affect the design. If merged I'll forward-port to master.

jtibshirani · 2021-08-31T23:01:33Z

server/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesFetcher.java

+ * Loads the mappings for an index and computes all {@link IndexFieldCapabilities}. This
+ * helper class performs the core shard operation for the field capabilities action.
+ */
+class FieldCapabilitiesFetcher {


This class lets us use the same per-shard logic for both the new and old execution strategies. It's not completely necessary to add it -- I could have shuffled some inner classes around to let us share this logic. However I found this to be a nice abstraction. It helps breaks up TransportFieldCapabilitiesAction, which is complex, and opens the door to adding unit tests for field caps (which I hope to in a follow-up).

jtibshirani · 2021-08-31T23:03:44Z

server/src/main/java/org/elasticsearch/action/fieldcaps/TransportFieldCapabilitiesAction.java

+                        List<ShardId> shardIds = entry.getValue();
+
+                        DiscoveryNode node = clusterState.getNodes().get(nodeId);
+                        assert node != null;


Can node be null? I didn't think so, but TransportFieldCapabilitiesIndexAction has some logic to handle this case:

elasticsearch/server/src/main/java/org/elasticsearch/action/fieldcaps/TransportFieldCapabilitiesIndexAction.java

Line 244 in 6a88d84

if (node == null) {

In general the logic around failures is pretty tricky. I'd like to write more tests for failure cases once I know the overall approach is okay.

I don't think it's a valid assertion. If the node is null, we should fail the request the same way since the node may vanished at any time. In the current model, that's ok because we'd try another replica but the new model requires to try these shards on potentially more than node. So unless we consider that we don't want to retry on replica, we'll need to handle failures differently.

We had a conversation offline where we decided the current strategy (where we don't retry failures) was not acceptable. For example if a shard is being relocated, the coordinator could have an out-of-date view of where it is, and the field caps request would fail. It's challenging for clients to handle these partial failures.

Maybe in an immediate follow-up, we can add support for a retry step, where we do another round of node requests to retry on other shards/ replicas. Like the other one around reducing responses, this follow-up would need to ship in the same release. For now in this PR I'd just replace the incorrect assertion with an error.

elasticmachine · 2021-08-31T23:41:00Z

Pinging @elastic/es-search (Team:Search)

dnhatn · 2021-09-08T02:55:42Z

Thanks @jtibshirani.

Previously when there was an index_filter, we tried each shard in the index one after the other, stopping when one of them matched. Now we fan out to all of the index shards preemptively, which means the nodes do redundant work.

I am not sure if it's a good approach because it will perform heavy works such as RBAC auth and iterating fields multiple times. As we retrieve field caps from any matching shard of an index, I think we can pass a list of indices instead in a node-level request. The receiving node then exhaustedly tries to all shards (one by one) of requesting indices until it finds matching copies. The coordinating node will retry on another node for indices without matching shards. WDYT?

jimczi

The logic makes sense to me. I left some comments regarding the merging of responses and the bwc.

jimczi · 2021-09-14T13:55:54Z

server/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesNodeResponse.java

+class FieldCapabilitiesNodeResponse extends ActionResponse implements Writeable {
+    private final String[] indices;
+    private final List<FieldCapabilitiesFailure> failures;
+    private final List<FieldCapabilitiesIndexResponse> indexResponses;


We could do a pre-merge locally to reduce the size of the response ? It seems wasteful to send back the entire list.

I agree, but I think this is nice to do as a follow-up to keep this PR simpler. Since it will affect the wire format, we should make the follow-up in this same release. I'll figure out how to do that (maybe we could have a short-lived feature branch).

jimczi · 2021-09-14T14:01:20Z

server/src/main/java/org/elasticsearch/action/fieldcaps/TransportFieldCapabilitiesAction.java


+        // If all nodes are on version 7.16 or higher, then we group the shard requests and send a single request per node.
+        // Otherwise, for backwards compatibility we follow the old strategy of sending a separate request per shard.


We have to take the remote cluster into account. Would it be simpler to make the decision on a per-connection level ? I think it's "ok" to emulate the old model by translating the node request into multiple shard request on older connections.

I'm having trouble understanding this part. Could you explain why we need to take the remote cluster into account?

jimczi · 2021-09-14T14:07:38Z

server/src/main/java/org/elasticsearch/action/fieldcaps/TransportFieldCapabilitiesAction.java

+        @Override
+        public void messageReceived(final FieldCapabilitiesNodeRequest request,
+                                    final TransportChannel channel,
+                                    Task task) throws Exception {


Should we fork the execution outside of the network thread ? The list of shards can be large on some tiers.

This is a good question! What do you think would be the right thread pool (maybe 'management')?

jimczi · 2021-09-14T14:19:46Z

server/src/main/java/org/elasticsearch/action/fieldcaps/TransportFieldCapabilitiesAction.java

+                        List<ShardId> shardIds = entry.getValue();
+
+                        DiscoveryNode node = clusterState.getNodes().get(nodeId);
+                        assert node != null;


I don't think it's a valid assertion. If the node is null, we should fail the request the same way since the node may vanished at any time. In the current model, that's ok because we'd try another replica but the new model requires to try these shards on potentially more than node. So unless we consider that we don't want to retry on replica, we'll need to handle failures differently.

jtibshirani · 2021-09-27T19:20:43Z

I discussed with @dnhatn and @jimczi and decided to merge the PR into a feature branch group-field-caps so we can follow up with more PRs. Some follow-ups we're tracking:

Handle shard failures through a retry mechanism
Reduce each node response before sending it back to coordinator
Move node-level logic to a separate threadpool (?)

We also discussed @dnhatn's concern about the strategy for index_filter. We thought it was the best trade-off to stick with the current strategy: it keeps the logic simple, and we're not too concerned about the extra work, as it is bounded by the number of nodes.

dnhatn

Thanks Julie! Let's merge this work to the feature branch.

This adds a retry mechanism for node level field caps requests introduced in #77047.

Currently to gather field caps, the coordinator sends a separate transport request per index. When the original request targets many indices, the overhead of all these sub-requests can add up and hurt performance. This PR switches the execution strategy to reduce the number of transport requests: it groups together the index requests that target the same node, then sends only one request to each node.

This adds a retry mechanism for node level field caps requests introduced in elastic#77047.

Currently to gather field caps, the coordinator sends a separate transport request per index. When the original request targets many indices, the overhead of all these sub-requests can add up and hurt performance. This PR switches the execution strategy to reduce the number of transport requests: it groups together the index requests that target the same node, then sends only one request to each node. Relates #77047 Relates # #78647 Co-authored-by: Julie Tibshirani <[email protected]>

elasticsearchmachine added the v7.16.0 label Aug 31, 2021

jtibshirani force-pushed the field-caps branch 3 times, most recently from a10af92 to 26427e9 Compare August 31, 2021 18:35

jtibshirani commented Aug 31, 2021

View reviewed changes

jtibshirani added 6 commits August 31, 2021 16:25

Pull out shard operation into its own class

97522d9

Add request + response classes, plus tests

999b7a5

Group index requests into node requests

48cb0af

Refactor collection to handle multiple responses per index

ae9db42

Also support grouping by node with index_filter

2ce1d68

Fix test failures

8b6b977

jtibshirani force-pushed the field-caps branch from 3b8023a to 8b6b977 Compare August 31, 2021 23:28

jtibshirani marked this pull request as ready for review August 31, 2021 23:32

jtibshirani added :Search/Search Search-related issues that do not fall into other categories >enhancement v8.0.0 labels Aug 31, 2021

elasticmachine added the Team:Search Meta label for search team label Aug 31, 2021

ywelsch self-requested a review September 1, 2021 11:19

jimczi reviewed Sep 14, 2021

View reviewed changes

jtibshirani added 4 commits September 23, 2021 14:05

Merge remote-tracking branch 'upstream/7.x' into field-caps

d681ae4

Return error when node is no longer available

5d95c5c

Skip over shard if we already found a match for that index

4efd602

Merge remote-tracking branch 'upstream/7.x' into field-caps

d34f016

jtibshirani changed the base branch from 7.x to group-field-caps September 27, 2021 18:45

dnhatn approved these changes Sep 27, 2021

View reviewed changes

jtibshirani removed v8.0.0 v7.16.0 labels Sep 27, 2021

jtibshirani merged commit 598ce64 into elastic:group-field-caps Sep 27, 2021

jtibshirani deleted the field-caps branch September 27, 2021 19:45

dnhatn mentioned this pull request Oct 5, 2021

Add retry for field caps node requests #78647

Merged

dnhatn added a commit that referenced this pull request Oct 14, 2021

Add retry for node level field caps requests (#78647)

6f31965

This adds a retry mechanism for node level field caps requests introduced in #77047.

dnhatn mentioned this pull request Oct 15, 2021

Add node-level field caps requests #79212

Merged

dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Oct 15, 2021

Add retry for node level field caps requests (elastic#78647)

557c1b0

This adds a retry mechanism for node level field caps requests introduced in elastic#77047.

dnhatn mentioned this pull request Oct 15, 2021

Add node-level field caps requests #79214

Merged

dnhatn mentioned this pull request Feb 24, 2022

Execute field caps per node directly on remote clusters #84348

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Group field caps shard requests per node #77047

Group field caps shard requests per node #77047

jtibshirani commented Aug 31, 2021 •

edited

Loading

jtibshirani commented Aug 31, 2021

jtibshirani Aug 31, 2021

jtibshirani Aug 31, 2021 •

edited

Loading

jimczi Sep 14, 2021

jtibshirani Sep 23, 2021

elasticmachine commented Aug 31, 2021

dnhatn commented Sep 8, 2021

jimczi left a comment

jimczi Sep 14, 2021

jtibshirani Sep 23, 2021

jimczi Sep 14, 2021

jtibshirani Sep 23, 2021

jimczi Sep 14, 2021

jtibshirani Sep 23, 2021

jimczi Sep 14, 2021

jtibshirani commented Sep 27, 2021

dnhatn left a comment


		// If all nodes are on version 7.16 or higher, then we group the shard requests and send a single request per node.
		// Otherwise, for backwards compatibility we follow the old strategy of sending a separate request per shard.

Group field caps shard requests per node #77047

Group field caps shard requests per node #77047

Conversation

jtibshirani commented Aug 31, 2021 • edited Loading

jtibshirani commented Aug 31, 2021

Choose a reason for hiding this comment

jtibshirani Aug 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Aug 31, 2021

dnhatn commented Sep 8, 2021

jimczi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtibshirani commented Sep 27, 2021

dnhatn left a comment

Choose a reason for hiding this comment

jtibshirani commented Aug 31, 2021 •

edited

Loading

jtibshirani Aug 31, 2021 •

edited

Loading